Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix abci_query RPC endpoint #77

Merged
merged 12 commits into from
Dec 2, 2019
Merged

Conversation

mappum
Copy link
Contributor

@mappum mappum commented Nov 27, 2019

This is a fix for #75, although it's not passing the integration test yet since Tendermint is responding with "0" for the height field but block::Height enforces that height is >= 1. Other unrelated integration tests are also failing, other RPC request/response types are possibly out of date or just overly strict.

@tarcieri
Copy link
Contributor

tarcieri commented Nov 27, 2019

@mappum I'm beginning to think it might be good to allow a Height of 0. It is representative of something meaningful: a chain in a state before it has produced an initial block, which in particular would help resolve this issue without making invasive changes to Tendermint KMS:

tendermint/tmkms#369

(also note I'm the one to blame for disallowing a height of 0, so I assume there's probably not going to be a lot of debate if you do change it as part of this PR)

@mappum
Copy link
Contributor Author

mappum commented Nov 27, 2019

@tarcieri I added the 0-height change.

However, it's still hard to use the integration tests because there are various differences between responses on different chains. It would be nice if it could be tested against an easy-to-spin-up local testnet (e.g. tendermint node --proxy_app=kvstore), since testing against gaiad has the external requirement of figuring out what network it should be on. Currently, gaiad for Cosmos mainnet is on an older version of Tendermint, so I'm not sure if there's some gaia testnet that the integration tests target.

@tarcieri
Copy link
Contributor

I added the 0-height change.

Great!

However, it's still hard to use the integration tests because there are various differences between responses on different chains.

Yeah it's been a big pain point in the past. If you can think of a better strategy (particularly one that works in CI) I'd be all for it.

@mappum
Copy link
Contributor Author

mappum commented Nov 27, 2019

I would vote for either making the integration test run something like this:

tendermint init --home=/some/tmp/dir
tendermint node --home=/some/tmp/dir --proxy_app=kvstore # (or counter or something)

(Then remove the Tendermint data after the tests run).

This could either run a tendermint binary included in this repo, or download it from Github releases (it could use the system's installed tendermint but this would mean people may run tests against the wrong version and have to worry about managing it themselves).

Until then, we could change the integration tests to target this kind of network but still expect the user to run it manually.

@tarcieri
Copy link
Contributor

@mappum there's a bit of prior art for this with the test harness used by the KMS:

You could potentially try to use the same Docker image on CircleCI?

@mappum
Copy link
Contributor Author

mappum commented Nov 28, 2019

@tony-iqlusion I can make that change in a later PR, for now I changed the integration tests to pass when using a local node for a throwaway network. Are you OK with merging this change? (I'd very much like to use this fix so I can use the RPC client).

@mappum mappum marked this pull request as ready for review November 28, 2019 00:01
tendermint/Cargo.toml Outdated Show resolved Hide resolved
@mappum
Copy link
Contributor Author

mappum commented Nov 29, 2019

I realized the encoding for the response should be changed too, since Tendermint always converts the key and value fields to base64 strings (but the request is in hex, lol).

Let me know if I should make any other changes.

tony-iqlusion
tony-iqlusion previously approved these changes Dec 1, 2019
tarcieri
tarcieri previously approved these changes Dec 1, 2019
@tarcieri
Copy link
Contributor

tarcieri commented Dec 1, 2019

@mappum looks like it needs a GPG signature

@mappum
Copy link
Contributor Author

mappum commented Dec 2, 2019

Signed last commit.

tarcieri
tarcieri previously approved these changes Dec 2, 2019
@tarcieri
Copy link
Contributor

tarcieri commented Dec 2, 2019

@mappum looks like it requires all commits to be signed. Perhaps squash and sign the resulting commit.

@mappum
Copy link
Contributor Author

mappum commented Dec 2, 2019

Made the changes, will wait to squash so it's easier to review.

@mappum
Copy link
Contributor Author

mappum commented Dec 2, 2019

Now all comments are addressed and all commits signed.

@tarcieri tarcieri merged commit 449fc98 into informalsystems:master Dec 2, 2019
@tarcieri
Copy link
Contributor

tarcieri commented Dec 2, 2019

:shipit:

@mappum mappum deleted the abci_query branch December 2, 2019 21:45
liamsi added a commit that referenced this pull request Dec 10, 2019
…p), and fmt:

 - #77 introduced an fmt issue
 - #36 was not up-to date regarding changes which were merged to master (renaming
   of simple_merkle)
 - new upstream release of made build of master fail (solution: explicitly add it
 ed25519-dalek (1.0.0-pre.3) with rand feature fixes that
@liamsi liamsi mentioned this pull request Dec 10, 2019
ebuchman pushed a commit that referenced this pull request Dec 10, 2019
…p), and fmt: (#83)

- #77 introduced an fmt issue
 - #36 was not up-to date regarding changes which were merged to master (renaming
   of simple_merkle)
 - new upstream release of made build of master fail (solution: explicitly add it
 ed25519-dalek (1.0.0-pre.3) with rand feature fixes that
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants